Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update release to build darwin/arm64 binary #5286

Merged
merged 5 commits into from
Feb 4, 2021

Conversation

briandealwis
Copy link
Member

@briandealwis briandealwis commented Jan 25, 2021

Towards #5042

We currently build with cgo to enable the fast notify trigger. But dockercore/golang-cross has too old a toolchain to enable cgo for darwin/arm64. So for darwin/arm64 we disable cgo and use notify's kqueue support instead.

This is a stop-gap measure since we need to produce signed binaries anyways (#2154).

This PR:

  • Tweaks the Makefile variables to allow specifying the Go version, build tags, and CGO_ENABLED on a per os/arch basis. Reorganize the variables on a per os/arch basis.
  • Updates github.com/rjeczalik/notify to track master to avoid explicitly specifying kqueue (Fix Darwin build without Cgo rjeczalik/notify#182) and also to pull in bug fixes that are not yet in a release.
  • Uses github.com/randall77/makefat to build a multi-arch ("fat") binary for Darwin for those that want it.

We currently build with cgo to enable the fast notify trigger.  But
dockercore/golang-cross has too old a toolchain to enable CGO for
darwin/arm64.  So for darwin/arm64 we disable CGO and use notify's
kqueue support instead.

- Tweak the Makefile variables to allow specifying the Go version,
  build tags, and CGO_ENABLED on a per os/arch basis.  Reorganize
  the variables on a per os/arch basis.
- Use github.com/randall77/makefat to build a multi-arch ("fat")
  binary for Darwin for those that want it.
@codecov
Copy link

codecov bot commented Jan 25, 2021

Codecov Report

Merging #5286 (c2040d5) into master (2588bd9) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #5286   +/-   ##
=======================================
  Coverage   71.84%   71.84%           
=======================================
  Files         392      392           
  Lines       14219    14219           
=======================================
  Hits        10215    10215           
  Misses       3256     3256           
  Partials      748      748           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2588bd9...c2040d5. Read the comment docs.

Makefile Outdated Show resolved Hide resolved
@briandealwis briandealwis marked this pull request as draft January 25, 2021 22:17
@briandealwis
Copy link
Member Author

Converting this to a draft as I'm not entirely sure if it's worth undertaking a stop-gap measure vs just doing the right thing in the first place.

@briandealwis
Copy link
Member Author

Converting this from draft: although using a CGO-less build is a stop-gap measure, it gets a native binary out. A perfect solution can be brought in with #2154.

@briandealwis briandealwis marked this pull request as ready for review February 2, 2021 20:03
Copy link
Contributor

@IsaacPD IsaacPD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was able to build the darwin/arm64 binary, don't have a machine to test the actual binary on though, LGTM!

@briandealwis briandealwis merged commit e348f56 into GoogleContainerTools:master Feb 4, 2021
@briandealwis briandealwis deleted the darwin-arm64 branch February 4, 2021 15:21
briandealwis added a commit that referenced this pull request Feb 8, 2021
We normally build with cgo to enable the fast notify trigger.  But
dockercore/golang-cross has too old a toolchain to enable CGO for
darwin/arm64.  So for darwin/arm64, disable CGO and use notify's
kqueue support instead.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants